Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize container launch priority performance #1490

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Optimize container launch priority performance #1490

merged 1 commit into from
Jan 23, 2024

Conversation

FillZpp
Copy link
Member

@FillZpp FillZpp commented Jan 22, 2024

Ⅰ. Describe what this PR does

Optimize container launch priority performance:

  1. Make the controller reconcile the same pod and barrier less times.
  2. Make the controller add the highest priority as soon as possible.
  3. Add completed annotation to avoid pod to be reconciled again if a running pod turns to not ready.

@kruise-bot kruise-bot added the size/L size/L: 100-499 label Jan 22, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 22, 2024 •
edited
Loading

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (30a660b) 48.40% compared to head (d2ac892) 48.61%.

Files Patch % Lines
...chpriority/container_launch_priority_controller.go 74.19% 11 Missing and 5 partials ⚠️
...tating/container_launch_priority_initialization.go 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1490      +/-   ##
==========================================
+ Coverage   48.40%   48.61%   +0.20%     
==========================================
  Files         157      157              
  Lines       22440    22460      +20     
==========================================
+ Hits        10862    10918      +56     
+ Misses      10387    10345      -42     
- Partials     1191     1197       +6
Flag Coverage Δ
unittests 48.61% <74.24%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@openkruise openkruise deleted a comment from kruise-bot Jan 22, 2024
furykerry
furykerry reviewed Jan 22, 2024
View reviewed changes
apis/apps/pub/launch_priority.go Outdated
@@ -29,4 +29,8 @@ const (
ContainerLaunchPriorityKey = "apps.kruise.io/container-launch-priority"
// ContainerLaunchOrdered is the annotation value that indicates containers in pod should be launched by ordinal.
ContainerLaunchOrdered = "Ordered"

// ContainerLaunchPriorityCompletedKey is the label indicates the pod has all its priorities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainerLaunchPriorityCompletedKey is an annotation not label

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

pkg/util/containerlaunchpriority/container_launch_prirotiy.go
@@ -17,6 +17,7 @@ limitations under the License.
package containerlaunchpriority
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file name containers a typo, plz rename it to container_launch_priority.go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

pkg/controller/containerlaunchpriority/container_launch_priority_controller.go
@@ -94,6 +93,30 @@ func add(mgr manager.Manager, r *ReconcileContainerLaunchPriority) error {
return nil
}

func shouldEnqueue(pod *v1.Pod, r client.Reader) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz add ut for shouldEnqueue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@FillZpp
Optimize container launch priority performance
d2ac892 
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
@FillZpp FillZpp force-pushed the optimize-container-launch-priority branch from 82cdc28 to d2ac892 Compare January 22, 2024 12:14
@kruise-bot kruise-bot added size/XL size/XL: 500-999 and removed lgtm size/L size/L: 100-499 labels Jan 22, 2024
furykerry
furykerry reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@zmberg
Copy link
Member

zmberg commented Jan 23, 2024

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot merged commit 17d8d4b into openkruise:master Jan 23, 2024
27 checks passed
@zmberg zmberg added this to the 1.6 milestone Mar 7, 2024
ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024
@FillZpp
Optimize container launch priority performance (openkruise#1490)
1f9a4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furykerry furykerry furykerry left review comments

@zmberg zmberg Awaiting requested review from zmberg

Assignees

@furykerry furykerry

@ls-2018 ls-2018

Labels
approved lgtm size/XL size/XL: 500-999
Projects
None yet
Milestone
1.6
Development

Successfully merging this pull request may close these issues.
5 participants
@FillZpp @zmberg @kruise-bot @furykerry @ls-2018